Skip to content

change internal time system of L1 products to be SCETime or UTC #447

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
nicHoch wants to merge 2 commits into
base: master
Choose a base branch
from
Open

change internal time system of L1 products to be SCETime or UTC #447

nicHoch wants to merge 2 commits into from

Conversation

@nicHoch
Copy link
Collaborator

@nicHoch nicHoch commented Dec 5, 2025

No description provided.

@nicHoch nicHoch requested a review from samaloney December 5, 2025 12:34
@nicHoch nicHoch self-assigned this Dec 5, 2025
@nicHoch nicHoch added this to the v1.7.0 milestone Dec 5, 2025
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 81.48148% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.04%. Comparing base (3ae9671) to head (9b9c9dc).
⚠️ Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
stixcore/products/product.py 86.95% 6 Missing ⚠️
stixcore/products/level1/quicklookL1.py 20.00% 4 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (3ae9671) and HEAD (9b9c9dc). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (3ae9671) HEAD (9b9c9dc)
2 1
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
- Coverage   72.23%   67.04%   -5.19%     
==========================================
  Files          78       76       -2     
  Lines        8085     8136      +51     
==========================================
- Hits         5840     5455     -385     
- Misses       2245     2681     +436

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

samaloney
samaloney reviewed Dec 11, 2025
View reviewed changes
Copy link
Collaborator

@samaloney samaloney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure allowing operations in different time systems is a good idea. A product should either have time in OBT or UTC and the conversion should only be done once in the from_level0 or similar methods?

All products (>L0) will always have obt_timerange from products will have an utc_timerange but only if the time sys in header is UTC and not sure supporting on the fly conversion is a good idea because the spice kernel dependence.

This aren't blocker just thoughts

@nicHoch
Copy link
Collaborator Author

nicHoch commented Jan 29, 2026

i agree that it is not ideal to provide some of the on the fly conversions. The auto converting most of the time (and if proper used never) will not happen as our process steps will do this explicit.

In general this L1 product could have state depended time systems and i think it would be bad experience if the properties will raise an exception if used

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samaloney samaloney samaloney left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@nicHoch nicHoch

Labels

None yet

Projects

None yet

Milestone

v1.7.0

Development

Successfully merging this pull request may close these issues.

3 participants

@nicHoch @codecov-commenter @samaloney